Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make Deployment PodTemplate not a pointer #16330

Merged
merged 2 commits into from
Nov 3, 2015

Conversation

mikedanese
Copy link
Member

No description provided.

@mikedanese mikedanese self-assigned this Oct 26, 2015
@k8s-github-robot k8s-github-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 26, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/XXL

@ncdc
Copy link
Member

ncdc commented Oct 27, 2015

@ironcladlou @kubernetes/rh-cluster-infra @liggitt @deads2k

@@ -208,7 +208,7 @@ type DeploymentSpec struct {
Selector map[string]string `json:"selector,omitempty"`

// Template describes the pods that will be created.
Template *api.PodTemplateSpec `json:"template,omitempty"`
Template api.PodTemplateSpec `json:"template"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we given up on ever having a TemplateRef? As I recall, that's why this is a pointer for ReplicationControllerSpec and I would expect congruence here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I discussed this with @bgrant0607 yesterday. The work was half done and no one is working on it or owns it. It's likely not going to get done before 1.2. Until that happens, generated documentation is confusing and incorrect (says this field is optional when it is actually not) and has been for months in the case of the ReplicationController. The change back can be made in conversions when we have a new API version.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we were to add TemplateRef, it would be in a new apiVersion, anyway. Making template a required field for now is more understandable for users.

TemplateRef is one of several issues to consider in the controller API overhaul: #14961. I don't know when I'll have time to get to it, though.

@mikedanese mikedanese force-pushed the deploy-ptr branch 2 times, most recently from 49f9fce to aec7b2f Compare October 28, 2015 07:10
@k8s-bot
Copy link

k8s-bot commented Oct 28, 2015

GCE e2e test build/test passed for commit 6bbc244.

@nikhiljindal
Copy link
Contributor

LGTM, thanks!

@nikhiljindal nikhiljindal added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2015
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Nov 3, 2015

GCE e2e test build/test passed for commit 6bbc244.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Nov 3, 2015
@k8s-github-robot k8s-github-robot merged commit 990c018 into kubernetes:master Nov 3, 2015
@mikedanese mikedanese deleted the deploy-ptr branch November 3, 2015 04:41
RichieEscarez pushed a commit to RichieEscarez/kubernetes that referenced this pull request Dec 4, 2015
…use deployments

added deployment-based e2e tests for horizontal pod autoscaling
adjusted to changes from PR kubernetes#16330
changed test titles according to PR comments & to merge change from PR kubernetes#16895
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants